Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Jetty12 + EE10 #2876

Draft
wants to merge 69 commits into
base: main
Choose a base branch
from
Draft

[WIP] Jetty12 + EE10 #2876

wants to merge 69 commits into from

Conversation

iamsanjay
Copy link
Contributor

@iamsanjay iamsanjay commented Nov 19, 2024

https://issues.apache.org/jira/browse/SOLR-17069

Currently, Jetty supports several environments:

  1. EE8 (Jetty10): Implements the Java EE8 specifications, supporting javax.servlet.
  2. EE9 (Jetty11): Implements Jakarta EE9 specifications, introducing the namespace migration from javax.* to jakarta.*.
  3. EE10 (Jetty12): Implements Jakarta EE10 specifications, including the Jakarta Servlet 6.0 specification, fully adopting the jakarta.* namespace.

Jetty 12 providing ongoing support for both older and upcoming EE specifications [from https://github.com/jetty/jetty.project/issues/10485]

In my opinion, Jetty12 + EE8 is the best next step. It retains compatibility with javax.servlet without forcing the removal of modules that haven’t transitioned to jakarta.* yet, while benefiting from all the new features introduced in Jetty12.

#1509

@iamsanjay
Copy link
Contributor Author

Recent commit fix the test cases related to V2 api. For now, it's just a workaround and I'll discuss with Jason to come up with a better solution. Earlier we were setting all the headers null, however the change in library does not provide one universal method removeHeader but separate methods to deal with all the headers.

Not valid anymore:

response.setHeader(name, null)

Changed to:

response.getHeaderNames().stream()
            .filter(name -> !name.equalsIgnoreCase("Content-Length"))
            .forEach(name -> response.setHeader(name, ""));
        response.setContentLength(-1);

@iamsanjay
Copy link
Contributor Author

Next, there is one issue with the InstrumentedHandler which is used to track metrics for JettyHandlers. However, with the upgrade that class is not valid anymore. Though dropwizard/metrics have newer version available for Jetty12 but that is only for EE10 (InstrumentedEE10Handler), nothing for EE8 AFAIK.

response.getHeaderNames().stream().forEach(name -> response.setHeader(name, null));
// Skip specific headers
// workaround for response.setHeader(name, null)
response.getHeaderNames().stream()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good for now IMO.

Longer term I'd love to see this code go away in favor of having the v2 API stuff get handled by its own servlet, but we're pretty far from that being workable for now. It is a little hacky in the interim though 😦

@fsparv
Copy link

fsparv commented Dec 4, 2024

In my opinion, Jetty12 + EE8 is the best next step.

does it matter that this is ee10 vs ee8 like the description?

https://issues.apache.org/jira/browse/SOLR-17503 (also linked from the ticket referenced above ticket). I don't have anything against continued support for javax per se, but there are definitely folks I've worked with that would benefit from being able to eliminate javax classes from their stack. Specifically they got tripped up by the appearance of jakarta based jetty versions (upgraded from other dependencies), which were then providing jakarta HttpServletRequests to tests based on our cloud solr test classes... boom. The goal with 17503 is to enable folks that integrate with our core code to use jakarta based servlet classes without explosion. If we do additional work to also continue to handle javax that's not a problem so long as Solr stops being the reason people can't upgrade their dependencies that also pull in jetty (or other jakarta based classes). One thing that's already sticky about our situation is our rest api stuff is already on jakarta packages ('jakarta.ws.rs:jakarta.ws.rs-api').

So IIUC, EE10 is simple to satisfy SOLR-17503 too, if we do more ok, but how important is that? is it worth the effort? I have a suspicion that we would involve separate artifacts and/or significant config gymnastics.

@iamsanjay
Copy link
Contributor Author

iamsanjay commented Dec 9, 2024

I picked changes from #2835 i.e. Hadoop-auth module being removed and this one and pushed a new branch to my fork.

Branch contains code for Jetty 12 + EE10 (Jakarta servlet api) https://github.com/iamsanjay/solr/tree/jetty12_ee10

I started working on ee8 because hadoop-auth module does not have any jakarta api support, however If #2835 will be merged then I believe we can transitioned to ee10 without any issue.

@epugh
Copy link
Contributor

epugh commented Dec 9, 2024

I picked changes from #2835 i.e. Hadoop-auth module being removed and this one and pushed a new branch to my fork.

Branch contains code for Jetty 12 + EE10 (Jakarta servlet api) https://github.com/iamsanjay/solr/tree/jetty12_ee10

I started working on ee8 because hadoop-auth module does not have any jakarta api support, however If #2835 will be merged then I believe we can transitioned to ee10 without any issue.

I've been sitting on merging the code, for no really good reason.. Thanks for highlighting that it's a bit of a blocker to your progress, I will update the PR and merge it today.

@epugh
Copy link
Contributor

epugh commented Dec 9, 2024

I picked changes from #2835 i.e. Hadoop-auth module being removed and this one and pushed a new branch to my fork.
Branch contains code for Jetty 12 + EE10 (Jakarta servlet api) https://github.com/iamsanjay/solr/tree/jetty12_ee10
I started working on ee8 because hadoop-auth module does not have any jakarta api support, however If #2835 will be merged then I believe we can transitioned to ee10 without any issue.

I've been sitting on merging the code, for no really good reason.. Thanks for highlighting that it's a bit of a blocker to your progress, I will update the PR and merge it today.

Merge is done!

@iamsanjay
Copy link
Contributor Author

c3c2f1c removed hadoop-auth
f7d900b is where the branch is upgraded to EE10, all the usage of javax.servlet has been replaced with jakarta.servlet

With this, changing the title of this PR as well.

@iamsanjay iamsanjay changed the title [WIP] Jetty12 + EE8 [WIP] Jetty12 + EE10 Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants